Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bybit: Fix WS ticker processing #1538

Merged
merged 2 commits into from
Jul 3, 2024

Conversation

gbjk
Copy link
Collaborator

@gbjk gbjk commented May 10, 2024

  • Fix spot ticker not sent to DataHandler
  • Fix no ticker updates sent to ticker system
  • Fix Options Low using Last field
  • Move ticker.Price Bid/AskSize fields out of "Funding Rates" section - They apply to options
  • Unify ticker structures for WS, Rest and asset types; They don't conflict
  • Unify WS ticker processing across assets
  • Remove ExtractCurrencyPair from tests
  • Add Test Coverage
  • Some test refactors

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How has this been tested

  • go test ./... -race
  • golangci-lint run
  • TestWsTicker

@gbjk gbjk added bug review me This pull request is ready for review labels May 10, 2024
@gbjk gbjk self-assigned this May 10, 2024
Copy link

codecov bot commented May 10, 2024

Codecov Report

Attention: Patch coverage is 82.55814% with 15 lines in your changes missing coverage. Please review.

Project coverage is 36.23%. Comparing base (06b9980) to head (22179fa).
Report is 6 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1538      +/-   ##
==========================================
- Coverage   36.28%   36.23%   -0.06%     
==========================================
  Files         419      419              
  Lines      183133   182999     -134     
==========================================
- Hits        66458    66308     -150     
- Misses     108578   108623      +45     
+ Partials     8097     8068      -29     
Files Coverage Δ
exchanges/bybit/bybit_types.go 66.66% <ø> (ø)
exchanges/ticker/ticker.go 90.39% <ø> (ø)
exchanges/bybit/bybit_websocket.go 46.04% <91.42%> (-0.52%) ⬇️
exchanges/bybit/bybit_wrapper.go 40.65% <43.75%> (-0.09%) ⬇️

... and 28 files with indirect coverage changes

Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the improvements! I found an issue earlier than this change though

exchanges/bybit/bybit_websocket.go Show resolved Hide resolved
exchanges/bybit/bybit_websocket.go Show resolved Hide resolved
exchanges/bybit/testdata/wsTicker.json Outdated Show resolved Hide resolved
exchanges/bybit/bybit_websocket.go Outdated Show resolved Hide resolved
exchanges/bybit/bybit_test.go Show resolved Hide resolved
@gbjk gbjk requested a review from gloriousCode May 15, 2024 07:39
Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, it all looks swell. Just a linter fix regarding 32bit

exchanges/bybit/bybit_test.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@shazbert shazbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff Tack!

exchanges/bybit/bybit_test.go Show resolved Hide resolved
exchanges/bybit/bybit_test.go Show resolved Hide resolved
@gbjk gbjk requested a review from gloriousCode July 2, 2024 05:59
Copy link
Collaborator

@gloriousCode gloriousCode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tACK! 😎

[DEBUG] | WEBSOCKET | 03/07/2024 15:37:02 | Bybit websocket connection: message received: {"topic":"orderbook.50.DOGEUSDT","ts":1719985022048,"type":"delta","data":{"s":"DOGEUSDT","b":[["0.12165","36664.7"]],"a":[],"u":7731147,"seq":43399847369},"cts":1719985022043}
[ERROR] | WEBSOCKET | 03/07/2024 15:37:02 | could not sync new data for Bybit DOG EUSDT spot Orderbook
[DEBUG] | WEBSOCKET | 03/07/2024 15:37:02 | Bybit websocket connection: message received: {"topic":"tickers.DOGEUSDT","ts":1719985022050,"type":"snapshot","cs":43399844543,"data":{"symbol":"DOGEUSDT","lastPrice":"0.1218","highPrice24h":"0.12555","lowPrice24h":"0.1209","prevPrice24h":"0.12393","volume24h":"101856596.5","turnover24h":"12578442.728828","price24hPcnt":"-0.0172","usdIndexPrice":"0.121624"}}
[INFO]  | SYNC | 03/07/2024 15:37:02 | Bybit websocket DOGE-USDT SPOT TICKER: Last 0.12180000 Ask 0.00000000 Bid 0.00000000 High 0.12555000 Low 0.12090000 Volume 101856596.50000000

@gbjk gbjk force-pushed the bugfix/bybit_spot_ticker branch from 22179fa to c954d85 Compare July 3, 2024 05:54
Copy link
Collaborator

@thrasher- thrasher- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR + in depth testing coverage!

@thrasher- thrasher- merged commit b7a2f61 into thrasher-corp:master Jul 3, 2024
5 of 11 checks passed
@gbjk gbjk deleted the bugfix/bybit_spot_ticker branch July 3, 2024 06:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug high priority review me This pull request is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants